Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Log error details via Display instead of Debug #249

Merged
merged 3 commits into from
Feb 17, 2018
Merged

Log error details via Display instead of Debug #249

merged 3 commits into from
Feb 17, 2018

Conversation

dhardy
Copy link
Member

@dhardy dhardy commented Jan 31, 2018

Compare current Debug output:

 WARN 2018-01-31T18:21:22Z: rand: OsRng failed [falling back to JitterRng]: Error { kind: Unavailable, msg: "missing", cause: None }
 WARN 2018-01-31T18:21:22Z: rand: JitterRng failed: Error { kind: Unavailable, msg: "timer jitter failed basic quality tests", cause: Some(NoTimer) }
thread 'main' panicked at 'could not initialize thread_rng: Error { kind: Unavailable, msg: "seeding a new RNG failed: both OS and Jitter entropy sources failed", cause: Some(Error { kind: Unavailable, msg: "missing", cause: None }) }', /home/dhardy/rust/rand/src/lib.rs:975:22

verses prettier Display output:

 WARN 2018-01-31T18:21:39Z: rand: OsRng failed [falling back to JitterRng]: RNG error [permanent failure]: missing
 WARN 2018-01-31T18:21:39Z: rand: JitterRng failed: RNG error [permanent failure]: timer jitter failed basic quality tests; cause: no timer available
thread 'main' panicked at 'could not initialize thread_rng: Error { kind: Unavailable, msg: "seeding a new RNG failed: both OS and Jitter entropy sources failed", cause: Some(Error { kind: Unavailable, msg: "missing", cause: None }) }', /home/dhardy/rust/rand/src/lib.rs:975:22

I checked implementations of Display for error types in the main Rust repo and all of them covered all their state I think; but none had a situation like this with optional cause. So this Display implementation seems reasonable to me.

(Note that the condition is a bit funky due to no_std not having a cause at all.)

@pitdicker
Copy link
Contributor

Ah, good use of Display!

I like "permanent failure or unavailable" more then "permanent failure", but shorter is a little neater here.
I don't see much use in adding "Rng error:" to the string. If I look at std::io::Error it also does not add such a text. What do you think of "{} ({}); cause: {}" as format string?

Example result:

 WARN 2018-02-01T18:02:36Z: rand: OsRng failed [falling back to JitterRng]: permanent failure (unexpected getrandom error); cause: Operation not permitted (os error 1)
DEBUG 2018-02-01T18:02:36Z: rand::jitter: JitterRng: testing timer ...
 WARN 2018-02-01T18:02:36Z: rand: JitterRng failed: permanent failure (timer jitter failed basic quality tests); cause: no timer available

@dhardy
Copy link
Member Author

dhardy commented Feb 4, 2018

Reminder to self: unwrap and expect on Result will display the error via Debug; this might want changing in a couple of places.

@dhardy dhardy mentioned this pull request Feb 4, 2018
Merged
@dhardy
Copy link
Member Author

dhardy commented Feb 5, 2018 •
edited
Loading

Specifically, check:

  • THREAD_RNG_KEY init uses expect
  • EntropyRng::fill_bytes uses unwrap
  • EntropyRng::try_fill_bytes logging

@dhardy
Copy link
Member Author

dhardy commented Feb 5, 2018

Updated; this affects EntropyRng logging significantly

pitdicker
pitdicker approved these changes Feb 6, 2018
View reviewed changes
Copy link
Contributor

@pitdicker pitdicker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look good to me! Have you made all the changes you wanted?

@dhardy
Copy link
Member Author

dhardy commented Feb 6, 2018

For now, though there might be some more tweaks after #252 merges.

@dhardy dhardy merged commit d45f06c into rust-random:master Feb 17, 2018
@dhardy dhardy mentioned this pull request Feb 17, 2018
Merged
@dhardy dhardy deleted the log-display branch March 14, 2018 16:40
pitdicker pushed a commit that referenced this pull request Apr 4, 2018
@dhardy
Merge pull request #249 from dhardy/log-display
61ffcd2 
Log error details via Display instead of Debug
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pitdicker pitdicker pitdicker approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dhardy @pitdicker